Support max_runtime in scheduler #6785
Conversation
berland
force-pushed
the
max_runtime
branch
2 times, most recently
from
9cfcd4d to
56b2ed2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6785 +/- ##
==========================================
+ Coverage 83.90% 83.91% +0.01%
==========================================
Files 365 365
Lines 21410 21428 +18
Branches 948 948
==========================================
+ Hits 17964 17982 +18
Misses 3152 3152
Partials 294 294 ☔ View full report in Codecov by Sentry. |
berland
added
scheduler
release-notes:skip
src/ert/scheduler/job.py
Outdated
| self.real = real | ||
| self.started = asyncio.Event() | ||
| self.returncode: asyncio.Future[int] = asyncio.Future() | ||
| self.aborted = asyncio.Event() | ||
| self._scheduler = scheduler | ||
| self._callback_timeout: Optional[Callable[[int], None]] = callback_timeout |
There was a problem hiding this comment.
Why is this function needed and do we need this function as an attribute?
There was a problem hiding this comment.
The timeout is set up by _legacy.py and we need to keep in sync with how the existing job_queue operates.
There was a problem hiding this comment.
Maybe we can have a
if isinstance(self._job_queue, Scheduler):in _legacy.py? I reckon that is less dirty than having this callback_timeout.
src/ert/scheduler/job.py
Outdated
| self.real = real | ||
| self.started = asyncio.Event() | ||
| self.returncode: asyncio.Future[int] = asyncio.Future() | ||
| self.aborted = asyncio.Event() | ||
| self._scheduler = scheduler | ||
| self._callback_timeout: Optional[Callable[[int], None]] = callback_timeout |
There was a problem hiding this comment.
Maybe we can have a
if isinstance(self._job_queue, Scheduler):in _legacy.py? I reckon that is less dirty than having this callback_timeout.
|
I added a commit that refactored the PR to use asyncio.wait_for, but I am not sure I liked that alternative better than the _max_runtime_task. |
src/ert/scheduler/job.py
Outdated
| threadsafer_future_wait(self.returncode), | ||
| timeout=self.real.max_runtime, | ||
| ) | ||
| except asyncio.exceptions.TimeoutError: |
When studying both approaches, I'm bit more inclined (since it cleaner tbh) towards having a separate task that just asyncio.sleeps and cancel jobs in the end. Also wait_for might be just another version of the same approach anyway. |
berland
force-pushed
the
max_runtime
branch
8 times, most recently
from
ab0c709 to
445d9d1
Compare
This highlights a behavioural change in the new LocalDriver, it will not send the same events as the legacy local driver, see test_async_queue_execution.py::test_happy_path The new scheduler will not catch bare exceptions for now, and thus the test for that situation is only applied for the legacy JobQueue.
Issue
Resolves #6784
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
Pre review checklist
Ground Rules),
and changes to existing code have good test coverage.
Pre merge checklist